[http-client-csharp] Fix BinaryContent wrapping in MethodParameterSegments navigation#10740
[http-client-csharp] Fix BinaryContent wrapping in MethodParameterSegments navigation#10740JonathanCrd wants to merge 3 commits into
Conversation
microsoft-github-policy-service
Bot
added
the
emitter:client:csharp
commit: |
JonathanCrd
requested review from
JoshLove-msft,
ShivangiReja,
christothes,
jorgerangel-msft,
joseharriaga,
jsquire and
m-nash
as code owners
|
No changes needing a change description found. |
JonathanCrd
force-pushed
the
joncarde/spector-streaming-jsonl
branch
from
952f63e to
e5cca81
Compare
JonathanCrd
changed the title
| Argument.AssertNotNull(info, nameof(info)); | ||
|
|
||
| ClientResult result = NoContentType(info.P2, info.P1, info.Action, cancellationToken.ToRequestOptions()); | ||
| ClientResult result = NoContentType(info.P2, info.P1, BinaryContent.Create(info.Action), cancellationToken.ToRequestOptions()); |
There was a problem hiding this comment.
What you have is not incorrect since it looks like this factory method eventually serializes the model with wire options, but we also already generate an implicit operator for mrw implemented models so what was here before was more intentional. Do you think we should scope down the change to only apply this for non model types and types that are supported by the BinaryContent.Create overloads?
There was a problem hiding this comment.
Fair point, let me try scoping it as you suggest
There was a problem hiding this comment.
After investigating, I found that the BinaryContent.Create(info.Action) change here is not caused by our fix — it's pre-existing regeneration drift. Regenerating Sample-TypeSpec with the current main generator (zero changes from this PR) produces the same BinaryContent.Create(info.Action) output.
Our fix only touches the MethodParameterSegments code path (line ~807), which already skips body params at line 793 (convenienceParam.Location == ParameterLocation.Body → continue). The NoContentType body goes through the separate body handling path (line 858+), which is untouched by this PR.
I've re-added the regen as a separate commit with a note that it's pre-existing drift, so CI passes. Both info.Action (implicit operator) and BinaryContent.Create(info.Action) (explicit factory) are functionally correct for model types — the implicit operator internally calls the same serialization path.
🤖 JonathanCrd's copilot
JonathanCrd
force-pushed
the
joncarde/spector-streaming-jsonl
branch
from
5bab1fe to
6ed8048
Compare
…n MethodParameterSegments navigation When MethodParameterSegments navigation resolves to a BinaryData property, the expression was passed directly as a BinaryContent protocol argument without wrapping. This caused a CS1503 compilation error. Wrap the resolved property expression with RequestContentApiSnippets.Create() when the protocol parameter is a content parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This change is pre-existing regeneration drift unrelated to the bug fix. The NoContentType convenience method now uses BinaryContent.Create() for the model-typed body property instead of the implicit operator. Both paths are functionally correct. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JonathanCrd
force-pushed
the
joncarde/spector-streaming-jsonl
branch
from
1ff399e to
c45b14f
Compare
3 participants
Summary
Fixes a generator bug where
MethodParameterSegmentsnavigation resolving to aBinaryDataproperty was passed directly as aBinaryContentprotocol argument without wrapping inBinaryContent.Create(), causing aCS1503compilation error.Bug Fix
When
ScmMethodProviderCollection.GetProtocolMethodArguments()navigatesMethodParameterSegmentsto resolve a property path (e.g.,stream→stream.Body), the resolvedBinaryDataexpression was passed directly as aBinaryContentprotocol argument. The fix wraps the expression withBinaryContent.Create()when the protocol parameter is a content parameter.This also fixed a pre-existing latent bug in the
Sample-TypeSpecNoContentTypemethod (info.Actionhad the same issue).Changes
ScmMethodProviderCollection.GetProtocolMethodArguments()to wrap withRequestContentApiSnippets.Create()when the protocol param is a content parameterSample-TypeSpec(side-effect of the fix)Validation
🤖 Created with JonathanCrd's copilot